fix: manualRun explicit-null completedAt, tri-state Restore gate, throttle honored in every mover flow (#394, #393, #374) - #411

… missing (#393)

…393)

The readiness gate's clear wrote `ReferentAvailable=True` and then let the
rest of the pass keep using the reconcile-start copy. The four gate parks in
`run_restore_mover` (MissingCaBundle, MissingServiceAccount, PrivilegedMover,
MissingCredentials) rebuild `conditions` from that copy and patch it
UNCONDITIONALLY, so they wrote the stale `False` straight back: two
resourceVersion-bumping writes per iteration, each waking the watch and
re-enqueueing, collapsing error-policy backoff into a tight loop for as long as
the missing Secret/SA/ConfigMap went unfixed — exactly the GitOps bring-up this
feature serves. Those parks are byte-identical no-ops today only because
nothing wrote conditions ahead of them.

`RepositoryGate::Proceed` now carries the `Cow<Restore>` the rest of the pass
must build on (borrowed when nothing was cleared, so the common path pays no
clone), and `reconcile_inner` rebinds `restore` to it — the caller cannot get a
`&Restore` to continue with without taking the carried one. The cleared-but-
stale combination has one construction site, the pure and total
`carried_after_clear`, so it is unwritable elsewhere.

Adds `KopiaClient::repository_set_client_read_only(bool)` →
`kopia repository set-client --read-only|--read-write`, the primitive a
read-only-connected source config needs to run a write-requiring verb:
flip read-write, run the verb, flip back.

Covered by a shim-binary unit test (exact argv, both polarities) and a new
`integration_set_client_throttle` test against the mise-pinned kopia 0.23.1,
which is the empirical gate for the dependent throttle milestones. It pins:

* set-client works on a `--readonly` connection and only rewrites the local
  config's `readonly` key;
* the flip is real, not cosmetic — `repository set-parameters` fails while
  read-only, succeeds inside the window, and fails again after the flip back;
* `throttle set` does NOT actually need the window. It succeeds directly on a
  read-only connection (it never puts a blob), contradicting the design premise
  this milestone was written to confirm. The flip is defensive there, not
  required; if a future kopia changes that, the test fails loudly;
* limits persist as `throttlingLimits` and survive the flip back;
* the load-bearing claim: `snapshot migrate --source-config` — which has no
  speed flags of its own — genuinely honors those config-persisted limits.

Two measured caveats the dependent milestones must design around, both
documented at the test's calibration table: throttling bites only on traffic
that reaches the backend (a warm source cache makes even a 1 KB/s cap
invisible), and kopia's throttler costs far more than its nominal rate implies
(28 KiB takes 14 s at a 2 MB/s cap; caps above ~10 MB/s do not bind at all).

)

dd23782 gave `RepositoryGate::Proceed` a `Cow<'a, Restore>`. A Cow stores its
owned variant INLINE, so the whole enum grew to a `Restore` while its sibling
variants are one `Action` each — `clippy::large_enum_variant`, and
`mise run clippy` (--workspace --all-targets -D warnings) exited 101.

Clippy's suggested `Box<Cow<..>>` would allocate on the BORROWED path too, and
that path is the common one this design deliberately keeps free. Instead
`CarriedRestore` boxes only its owned arm:

    Unchanged(&'a Restore)   // no clone, no allocation
    Cleared(Box<Restore>)    // one Box atop a clone already being made

so the gate enum is pointer-sized in both arms. Semantics are unchanged: the
no-clone fast path, the 'caller cannot get a &Restore without taking the carried
one' property, and the single total construction site all survive, and `get()`
is an exhaustive match so a third carrier shape must decide which object the
pass continues with.

The guard test now pins the carrier variant in both arms; its mutation-detection
property was re-verified after the refactor.

…ad-only (#374)

Review finding: the test proved every piece of the source-side sequence but
never asserted the invariant the pieces exist to protect. Since `throttle set`
turns out NOT to need the read-write window, what M3c/M3d will ship is
`connect --readonly` -> `throttle set` -> migrate with no flip — and
`throttle set` rewrites the config file. If that rewrite silently dropped the
`readonly` key, a replication SOURCE would be left writable and nothing
downstream would notice: the next step flipped read-write on purpose, and the
step after re-established the bit explicitly.

Restructures so the measured path IS the production path:

* the cap now lands at step (1b) on the read-only connection, and step (1c)
  immediately asserts the bit survived — in the config JSON AND behaviorally,
  via a write-requiring `set-parameters` probe;
* steps (2)-(4) keep the flip proof for the verbs that genuinely need the
  window, and (5) asserts the cap set on the read-only connection came through
  the flip sequence untouched, so (6) times the config the no-flip sequence
  wrote;
* the timing bound is now self-calibrating: an identical UNTHROTTLED baseline
  migrate is timed first and the capped run must be >= 10x it, so the verdict
  no longer depends on machine speed. The 3s absolute floor stays as a backstop
  against a pathologically slow baseline making the ratio trivial.

Both new assertions were mutation-checked against the real binary: forcing the
read-only bit off before (1c) fails it with "the config must record
`readonly: true`", and a non-binding 20 MB/s cap fails the ratio bound at
190.5ms vs a 189.9ms baseline.

Also from the review ledger: pin "set-client writes nothing to the backend" by
fingerprinting every blob across the flip instead of asserting it in prose, and
truncate the shim's argv capture directory before use so an appending shim
cannot inherit stale lines.

)

`repository throttle set` ran only in `run()`'s generic connect arm, so every
flow that opens its OWN connection silently dropped the `spec.throttle` its
controller had faithfully put on the wire: bootstrap (all three connects,
including the `mode: ReadOnly` probe), seed-migrate's local repository,
maintenance, verify and replicate. kopia's limits are per-CONNECTION — they
persist into that connection's client config — so a second connect under a
second config needs its own call; there is nothing repository-wide to inherit.

The invariant this ships: EVERY mover flow that connects a repository applies
`MoverWorkSpec.throttle` immediately after its connect, no exceptions. One
shared `apply_repository_throttle` helper does it, skipping when no limit is
set (no kopia process spawned for the common case), treating a refusal as
terminal (running uncapped over the link the user capped is worse than not
running), and emitting the one `applied repository throttle` line that is both
the e2e-assertable proof and the old-mover/new-controller skew signal.

Read-only connections are throttled too. M3a measured `repository throttle set`
succeeding on a `--readonly` connect against kopia 0.23.1 (with `set-parameters`
as the control probe that does hard-error there), so the read-write flip is not
needed and `repository_set_client_read_only` stays an unwired contingency.

The dispatch `_ =>` catch-all in `run()` is replaced by an exhaustive
`Operation` match — that catch-all is how this bug shipped, and a new operation
must now decide its throttle story to compile. Bootstrap's three connects go
through one funnel that applies the throttle by construction; a throttle failure
there is kept structurally distinct from a CONNECT failure (`BootstrapConnectFailure`),
since the probe reads a connect failure to decide whether to CREATE or SEED a
repository over the backend and a throttle failure says nothing about it.

Deliberately out of scope, both stated rather than forgotten:
- snapshot delete-batch and pin work specs carry `Default` (empty) throttle
  from their controllers, so they ride the generic arm as a no-op — a
  controller-side gap, not a mover one;
- browse builds its work spec in the CLI with `throttle: Default::default()`,
  so its `spec.throttle` is structurally always empty (comment only);
- SnapshotReplicate's dual connect and the seed SOURCE (replica) connect land
  with their own per-CR override surfaces in the following commits.

E2e, no silent green: three fixture repositories gain a non-binding 100 MiB/s
`moverDefaults.throttle` and the scenarios assert the mover's own log line —
timing proves nothing, since kopia's cap only bites cold-cache backend traffic.
Covers bootstrap + maintenance (S3 maintenance scenario), verify, and replicate;
each pre-created repository is read back so a dropped overlay cannot make the
assertion vacuous.

…#374)

Review follow-up to 869cf8f. Two findings, both about the fix being remembered
rather than enforced.

**The six own-connect flows were still remembered call sites.** Deleting
`apply_repository_throttle` from `run_verify_flow` compiled and passed every
hermetic gate — the run just went uncapped — which is the structure that caused
#374 in the first place, one layer up. Replaced with a `connect_and_throttle`
funnel (mirroring the `bootstrap_connect` one this PR already added): the
generic `run()` arm, maintenance, verify, replicate and a seed's local
repository now obtain a connection and its cap as ONE operation, so a future
flow cannot get the first without the second. Its `ConnectFailure` keeps the
connect and throttle halves distinct and labels the connect half with the
calling flow's own `KopiaOp`, so a status block still names the invocation that
actually failed.

Nothing in the type system stops a new flow from calling `repository_connect`
directly, so `connect_funnel_is_the_only_way_flows_connect` ratchets the count
of direct connects left in the file against a documented allowlist (the two
funnels plus the four connects that genuinely carry no `MoverWorkSpec.throttle`:
serve, browse, snapshot-replication's dual connect, a seed's source). Verified
to fire: adding one bypassing connect fails it 10 vs 9.

**Seed-migrate's local connect had no coverage of any kind** — no hermetic test
reaches `seed_create_and_connect_local`, and the e2e table covered
bootstrap/maintenance/verify/replicate only, so the highest-value call site of
the change ("the heaviest transfer kopiur performs") could have been deleted
and stayed green forever. The migrate-seed scenario now sets
`moverDefaults.throttle` (read back, so a dropped overlay cannot make it
vacuous) and asserts the mover's log line appears at least TWICE in a SINGLE
pod: a seeding bootstrap caps two connections (the seed's own local kopia config
— the only cap `snapshot migrate` can honor — and the post-seed reconnect).
Per-pod counting is load-bearing: the same selector also matches later
catalog-rescan pods that log it once each, so a bare `contains`, or a count
across pods, would pass with the seed-local call removed. `wait_for_pod_log` is
now the `times = 1` case of `wait_for_pod_log_times`.

Also, from the review's minor ledger: the helper's doc no longer claims a
no-exceptions invariant it cannot hold — it carries the exception list with each
reason and owning milestone; the generic arm's throttle failure now flows into
`result` instead of returning early past `metrics.record`/`shutdown`; and the
pod-log helper retains the last per-pod log error so a poll that times out
reports why rather than just "not found".

)

`kopia snapshot migrate` has no speed flags, and kopia's limits are
per-CONNECTION — so a snapshot replication, which opens two repositories
under two kopia configs, needs two independently resolved caps. Before
this, only the source connection was even reachable by a throttle (via
the work spec's `throttle`, itself only just wired), and the destination
— the off-site link that is usually the scarce one — had no cap surface
at all.

- API: `MigrateOptions.throttle: Option<MigrateThrottle>` with
  `source`/`destination` sub-blocks reusing `Throttle`. `MigrateOptions`
  loses `Copy`. Admission rejects a non-positive rate on either side,
  naming the side and the knob (a shared `validate_throttle` helper).
- Controller: `io::merged_throttle` resolves ONE side — that repository's
  `moverDefaults.throttle` overlaid by the CR's override FIELD BY FIELD,
  so pinning one knob never drops the repository's others. Called twice;
  the destination block rides the op as `destinationThrottle`.
- Mover: both srepl connects apply their own block immediately after
  connecting, each terminal via `srepl_terminal` — proceeding uncapped
  would saturate exactly the link the user asked to protect. The source
  application lands on the read-only connect (no read-write flip: kopia
  accepts `throttle set` there, pinned by the kopia integration test).
- Tests: the controller-glue guard now carries per-side overrides over
  two repositories that BOTH set defaults, asserting the merge and that
  neither side's knobs cross into the other; merge unit tests; wire-shape
  + old-wire-decode for `destinationThrottle`; camelCase roundtrip.
- E2e: the full-history scenario throttles both fixture repos and both
  sides of the CR, reads the CR back to prove the new field survived
  admission, asserts both merged blocks on the wire, and requires the
  srepl pod to log `applied repository throttle` twice — one occurrence
  means one side's cap is gone.
- Docs: per-side semantics and precedence, why migrate has no speed
  flags, the measured caveats (byte caps are far more punitive than
  nominal on small-object cold-cache work; warm reads bypass throttling
  entirely), and the helm-`crds/`-is-install-only trap that silently
  prunes new fields.

#374)

The reworked `moverDefaults.throttle` bullet enumerated the movers that
honor it and listed `deletion` — but the SnapshotDeleteBatch and
SnapshotPin work specs are built with `throttle: Default::default()`
unconditionally, so the generic mover arm applies nothing for them. A
precise-looking enumeration that is wrong is worse than the vague claim
it replaced: someone capping a repository to protect an object store
would not get that cap on bulk deletes, and nothing would say so.

Deletion moves to the exceptions, which are now their own paragraph
(serve/browse, a RepositoryReplication's destination side, batched
deletions and pins), each with its reason, and deletion's actual bound
named — per-repository batching plus `maxConcurrentDeleteJobs` — so the
gap reads as scoped rather than unqualified. The builders are left
alone; populating them stays out of scope.

Two adjacent cleanups:

- `srepl_terminal` logs `class` for a `MoverError::Kopia` the way
  `srepl_terminal_kopia` already does, so whether an operator can filter
  on it no longer depends on which helper a call site picked.
- The throttle integration test's prose said M3c/M3d "will ship" the
  read-write flip sequence. They didn't: the flip was empirically shown
  unnecessary and `repository set-client` stays unwired. The test now
  says so and states its own role as a contingency-only guard.

A seed is the single heaviest transfer kopiur performs — a whole
repository pulled across a link on the worst day you have had all year —
and migrate mode had no cap surface at all. Blob mode at least offered
`sync-to`'s speed flags; a migrate seed offered nothing, because `kopia
snapshot migrate` carries no speed flags either. The only lever is
`repository throttle set` on each CONNECTION, and a migrate seed opens
two under two kopia configs: the replica it reads and the repository it
writes.

- API: `SeedMigrateOptions.throttle: Option<MigrateThrottle>`, reusing
  the same per-side {source, destination} block SnapshotReplication's
  `spec.migrate.throttle` uses — here `source` is the REPLICA named by
  `seed.from.repository` and `destination` is this repository.
  `SeedMigrateOptions` loses `Copy`. Admission rejects a non-positive
  rate on either side, naming the side and the knob (the shared
  `validate_throttle`); a throttle on a BLOB seed stays refused by the
  existing mode-pairing rule, as an inert field.
- Controller: the replica's block is resolved WITH the source it belongs
  to, inside `seed_op_for`, so the two can never describe different
  repositories — that repository's own `moverDefaults.throttle` overlaid
  field-wise by the CR's override, and empty in blob mode. It rides the
  op as `replicaThrottle`. This repository's side rides the bootstrap
  work spec's `throttle`, now `seed_bootstrap_throttle`: the same merge,
  gated on the seed being ARMED so a number chosen for a one-time copy
  does not go on capping every later connect. Both repository kinds take
  the resolved block instead of `moverDefaults`, so they cannot drift.
- Mover: `seed_connect_source` applies the block in place immediately
  after the read-only replica connect (no read-write flip — kopia accepts
  `throttle set` there), BEFORE the persisted-password probe, so the
  probe sees the config `snapshot migrate` will reopen in its final
  state. Terminal, like every other application.
- Tests: a seed glue guard over two repositories that BOTH set defaults,
  asserting each side's partial override merges and that neither side's
  knobs cross into the other, plus the unarmed and blob arms; wire shape
  + old-wire decode for `replicaThrottle`; per-side validation; a
  camelCase round-trip through the apiserver path.
- E2e: the migrate scenario now throttles the replica repo, the seeded
  repo and both sides of the CR, reads the CR back to prove the new
  fields survived admission, and requires the seeding pod to log
  `applied repository throttle` THREE times — replica, seed-local,
  post-seed reconnect (the probe connect fails on an uninitialized
  backend, which is what arms the seed, so it never reaches its own).
- Docs: per-side semantics and precedence, why migrate has no speed
  flags, why the override is armed-only, the measured caveats (byte caps
  bite far harder than nominal on small-object cold-cache work; warm
  reads bypass throttling), and the helm-`crds/`-is-install-only trap
  that silently prunes new fields.

…374)

Both described the world before the replica connect was capped, in the two
places a reader is most likely to trust them.

- `seed_create_and_connect_local` still said the seed SOURCE's cap was
  "wired in a later change" — this WAS that change, so the comment told a
  reader of the local connect that the replica runs uncapped. It now names
  `replicaThrottle` and where it is applied.
- `apply_repository_throttle`'s doc used a seed as its worked example of
  "one line per APPLICATION" and put the count at two connections (the
  bootstrap's and the local repository's). A migrate seed now opens three,
  and the e2e asserts three. Restated as the general rule plus the exact
  seed arithmetic: three for a FIRST seed — the replica, the seed-local
  client and the post-seed reconnect, its probe connect having failed on
  the uninitialized backend, which is what armed the seed — and four when
  resuming, where that probe connect succeeds and is capped too.

Same off-by-one in docs/repositories.md's grep hint, which claimed three
unconditionally; it now distinguishes the first seed from a resume.

Comments and prose only; no behavior change.

…rose (#374, #393, #394)

Four small, behavior-neutral findings from final review:

- crates/mover/src/main.rs: remove a doc-comment paragraph pair duplicated
  back-to-back on apply_repository_throttle.
- crates/mover/src/main.rs: reword the maintenance/verify/replicate-source
  error! log literals, which also fire on a refused throttle set
  (ConnectFailure::Throttle) and previously read as pure connectivity
  failures. No change to error classes, propagation, or status bodies.
- docs/dev/api-conventions.md: document the manualRun.completedAt exception
  to the blanket skip_serializing_if convention (#394).
- crates/controller/src/restore/tests.rs: fix a stale doc comment referring
  to Cow<Restore>; the type is CarriedRestore since 7c89887 (#393).
- docs/reference/crds/shared-types.md: add "seed" to the throttle
  honored-flows list, consistent with the seed-shape sentence that follows.

#396 follow-up)

A repository that finished a seed span its own reconciler at ~4 passes a
second until its bootstrap Job's TTL expired — a self-sustaining hot loop
that starved every other Repository in the cluster.

`finalize_bootstrap` re-enters on EVERY reconcile for as long as the
finished bootstrap Job exists: the Job is deleted only on the health-probe
arm, and a seeding bootstrap is never a probe run. Each re-entry re-reads
the same result ConfigMap, so `result.seed` is still the same
`SeedOutcome`, forever — and `seed_success_fold` stamped
`status.seed.seededAt = Utc::now()` unconditionally. A fresh timestamp on
every pass meant `status_patch_is_noop` never held, the status PATCH bumped
`resourceVersion`, the controller's own primary watch re-delivered the
object as `object.reason="object updated"`, and round it went. The
`RepositorySeeded` Event is gated on that same write, so the loop also
minted one Event aggregation per pass.

A second, independent defect kept the loop alive even with a stable clock:
the no-op guard compares the whole `seed` value, while the fold wrote a
PARTIAL sub-object that omitted the `startedAt` marker `seed_marker_patch`
had stamped. A partial object can never equal the stored one, so the write
repeated regardless of the timestamp.

- `seed_success_fold` now takes the live `status.seed`. `seededAt` is
  stamped ONCE, at the first fold, and reused verbatim afterwards — a seed
  happens exactly once, so its completion time is a fact, not a heartbeat
  (the rule `seed_marker_patch` already applies to `startedAt`). The marker
  itself is carried forward into the patch so the fold's `seed` object
  equals the stored one byte for byte. Both repository kinds pass their own
  `status.seed`, so neither can drift.
- Regression test: re-folding a finished MIGRATE seed minutes later must be
  a `status_patch_is_noop`, shaped like the e2e's `e2e-seed-mig-dst`. It
  covers both arms — the advancing wall clock and the missing marker — and
  pins that the FIRST fold still stamps `now`. Reverting either half of the
  fix fails it.

This predates the branch: the unguarded `seededAt = now` arrived with
74665dc (#380 / #396) and reproduces identically at the merge-base of main.
It only went red now because the consolidated e2e run puts the migrate-seed
scenario (which deliberately leaves its seeded Repository standing) in the
same cluster as `ready_repository_steady_state_is_quiet`, which counts
fleet-wide Repository reconciles.

The shared `status_patch_is_noop` shallow-compare stays as it is; making it
a recursive-subset compare is a global guard-semantics change and is
deferred to its own review.

…394)

The #394 e2e demanded the stale `completedAt` key be ABSENT after the
non-terminal patch nulled it. Against a real apiserver it is not: on
k8s 1.33 a nullable CRD field answers a merge-patch null by STORING the
null rather than deleting the key. The scenario failed on a property it
never meant to assert.

#394's semantics are correct either way — an absent key and an explicit
null both decode to `None`, both re-serialize to `null`, and neither
leaves the previous run's timestamp standing, which is the whole point.
Only the assertion and the prose were over-specified.

- E2e: assert there is no completedAt VALUE (absent OR explicit null is
  fine; a non-null string is the bug), with the message rewritten to name
  that property instead of the deletion mechanic.
- Comments at both serde sites (`common::ReplicationManualRunStatus`,
  `maintenance::ManualRunStatus`) and in the api/controller tests now say
  the apiserver either deletes the key or stores the null, and why both
  converge. The `Patch::Merge`-not-`Patch::Apply` dependency is unchanged
  and still called out.
- `manual_run_patch_converges_after_clearing_a_stale_completed_at` now
  replays BOTH apiserver outcomes against the noop guard, so the stored-null
  shape is pinned rather than assumed.